-
Notifications
You must be signed in to change notification settings - Fork 349
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement file metadata, read, and write APIs. #4043
base: master
Are you sure you want to change the base?
Conversation
This also improves how HANDLEs are, well, handled a bit. STDIN/ERR/OUT are now proper pseudo-handles. The invalid handle is added and encode/decodes to exactly 0xFFFFFFFF correctly. Unix shims require changes to hoist FdTable up, so that Windows can share most of its implementation.
Thanks for the PR! This is a huge PR. It will take a while until I have enough time to take a look at such a big change. If there is any way to stage this as a series of smaller changes, that would be much appreciated. |
I could possibly further split this down by first PRing the |
That sounds like a good plan. You can keep this PR open (maybe mark it as draft); it can serve as good context for why you want to refactor things the way you suggest. |
Also, I know very little about the Windows API... @ChrisDenton @beepster4096 if you have any chance at taking a look at the windows-specific parts here and check whether they look sensible, that would be great. :) |
@sivadeilra - here's one of the PRs related to adding Miri support for Windows FS APIs, RE: #3482 |
☔ The latest upstream changes (presumably #4046) made this pull request unmergeable. Please resolve the merge conflicts. |
Stdin, | ||
Stdout, | ||
Stderr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the handles returned by GetStdHandle
are proper file handles and not pseudo-handles. This differs from posix where the fds have fixed values. I realise that miri does want to track the difference but I worry about confusing the terminology.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, some ideas on paths forwards:
- Leave it as-is, but add documentation about the terminology here
- Try to merge stdin/out into
Handle::File
as normalFileDescription
(basically what unix does)- This will require some more changes to the
FileDescription
trait. Itsread
andwrite
methods are a bit tailored towards unix flavor currently, you may notice I have some hacky code in here to work around that. I'd like to fix it eventually anyways, but it adds more friction to MVP
- This will require some more changes to the
- Make
Handle::File
hold an enum likeenum FileHandle { Stdin, Stdout, File(i32) }
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its read and write methods are a bit tailored towards unix flavor currently, you may notice I have some hacky code in here to work around that. I'd like to fix it eventually anyways, but it adds more friction to MVP
Is it even worth using the same trait for Unix and Windows? The trait is indeed very tailored towards Unix, deliberately so. Knowing nothing about the Windows APIs, I expected they would need their own trait anyway.
I haven't looked yet at those hacks. Can you write a high-level summary of the pros and cons of using the same vs a different trait?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, the main pro of reusing the trait at all is because we need a common base trait so long as we don't intend to have two different 'open file maps', one for Windows-like systems and one for Unix-like systems. And most of the FdMap
code works well for this purpose - map of numbers to open files is actually a pretty good abstraction for both platforms. The read/write/seek parts of the trait aren't very file-system dependent already, as they're closer to abstractions over the File
interface than the syscalls.
To use an entirely different trait, we'd have to somehow support two different dyn Trait
in the file code. They also actually would be nearly identical - I think I can remove the main hack I was talking about, looking at it again with fresh eyes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expected we'd have two different open file maps, yeah.
But if there's stuff that can be usefully shared between Windows and Unix, even better! I just want to make sure we can keep evolving the Unix side without being held back by constraints or lack of knowledge about what happens on the Windows side.
This also improves how HANDLEs are, well, handled a bit. STDIN/ERR/OUT are now proper pseudo-handles. The invalid handle is added and encode/decodes to exactly 0xFFFFFFFF correctly.
Unix shims require changes to hoist FdTable up, so that Windows can share most of its implementation.